ext/spl: Narrow tentative return types for DirectoryIterator methods - #22642
ext/spl: Narrow tentative return types for DirectoryIterator methods#22642arshidkv12 wants to merge 3 commits into
Conversation
|
|
||
| CHECK_DIRECTORY_ITERATOR_IS_INITIALIZED(intern); | ||
| RETURN_LONG(intern->u.dir.index); | ||
| RETURN_STRINGL(intern->u.dir.entry.d_name, strlen(intern->u.dir.entry.d_name)); |
There was a problem hiding this comment.
Why did you change the return data here? It's BC break.
There was a problem hiding this comment.
I changed it to match the TODO in the stub, but you're right—it changes the runtime behavior and is a BC break.
There was a problem hiding this comment.
just few things.
- It breaks the "agreement" between
spl_filesystem_dir_current_keythat uses the integer index and this ::key() method. Maybe the stub should be fixed as to return int ? - If you want the current entry name, it seems
::getBasename()(::getFilename()eventually ?) is already doing it.
There was a problem hiding this comment.
class FilesystemIterator extends DirectoryIterator{
public function key(): string {}
}
class DirectoryIterator extends SplFileInfo implements SeekableIterator{
public function key(): int {}
}How to solve it?
There was a problem hiding this comment.
class FilesystemIterator extends DirectoryIterator{ public function key(): string {} } class DirectoryIterator extends SplFileInfo implements SeekableIterator{ public function key(): int {} }How to solve it?
then int|string might work (current() would be string|SplFileInfo) ?
| */ | ||
| public function current(): mixed {} // TODO narrow return type | ||
| /** @tentative-return-type */ | ||
| public function current(): string|SplFileInfo|RecursiveDirectoryIterator {} |
There was a problem hiding this comment.
thanks for taking care of these TODOs, I wrote them back then :)
A don't understand the RecursiveDirectoryIterator here. why is it listed in the return type?
P.S. the new type should also be compatible with FilesystemIterator::current() which is string|SplFileInfo|FilesystemIterator (but what should rather be string|SplFileInfo|static in fact).
So maybe the return type of Directory::current() should be string|SplFileInfo|static? (please double check it, it's just my assumtion)
There was a problem hiding this comment.
And then the return type of FilesystemIterator::current() should also be fixed
ext/spl: Narrow tentative return types for DirectoryIterator methods Add tentative return type to Directory::current() Add tentative return type to Directory::current() Add tentative return type to Directory::current()
4 participants
Update the
DirectoryIteratorstubs to narrow the tentative return typesof
key()andcurrent(), and regenerate the generated arginfo.